Code Review: Fix item-at Satisfies source typing (#5706)

The fix is correct and clearly addresses the root cause: Satisfies was constructing a ValueAssertion<TItem> (a bare wrapper over IAssertionSource<TItem>) rather than a richly-typed assertion source, so users of collection-type items lost all the specialized assertion API.

Architecture concern: combinatorial overload explosion

ListItemAtSatisfiesExtensions.cs is 311 lines covering 11 concrete collection types × 2 source types = 22 overloads. This approach works, but it won't scale. Every time a new assertion type is added to TUnit (e.g. SortedSetAssertion, ImmutableArrayAssertion, or any user-defined collection assertion type), this file must be updated with 2 more overloads. The pattern also doesn't compose — it says nothing about IList<IList<IList<T>>> or other nested structures.

A more maintainable design would expose a single generic overload that accepts a source factory:

// Single entry point — no enumeration of concrete types needed
public static ListItemAtSatisfiesAssertion<TList, TItem> Satisfies<TList, TItem, TSource>(
    this ListItemAtSource<TList, TItem> source,
    Func<TItem, string, TSource> sourceFactory,
    Func<TSource, IAssertion?> assertion,
    [CallerArgumentExpression(nameof(assertion))] string? expression = null)
    where TList : IList<TItem>
    where TSource : IAssertionSource<TItem>
{
    // one implementation
}

Usage becomes:

await Assert.That(items).ItemAt(0)
    .Satisfies(
        (item, label) => new CollectionAssertion<int>(item, label),
        a => a.Count().IsEqualTo(3));

This pushes the factory responsibility to the call site, but TUnit could still ship convenience helpers (e.g. static factory methods on each assertion type) rather than enumerating every combination. The key benefit: users can bring their own assertion source types and they'll work automatically — the current approach gives them no way to do that.

internal accessors as a design smell

// ListAssertions.cs (and ReadOnlyListAssertions.cs)
internal AssertionContext<TList> InternalListContext => _listContext;
internal int InternalIndex => _index;

These were private before; they're now exposed internal solely to allow the extension class (in the same assembly) to bypass normal construction. The Internal prefix signals this awkwardness. A cleaner alternative: a factory/builder method directly on ListItemAtSource<TList, TItem> that creates the assertion given a raw factory:

internal ListItemAtSatisfiesAssertion<TList, TItem> CreateSatisfiesAssertion(
    Func<TItem, int, IAssertion?> factory)
{
    _listContext.ExpressionBuilder.Append(...)
    return new ListItemAtSatisfiesAssertion<TList, TItem>(_listContext, _index, factory);
}

This hides the internals and keeps the responsibility for mutating ExpressionBuilder inside the source type, where it belongs.

The internal assertion constructor is unguarded on the expression side

The new internal constructor of ListItemAtSatisfiesAssertion skips the expression-appending that SatisfiesSpecialised does manually:

// Extension helper appends expression here...
source.InternalListContext.ExpressionBuilder.Append($".Satisfies({expression})");

// ...then constructs via internal ctor which does NOT append anything
return new ListItemAtSatisfiesAssertion<TList, TItem>(
    source.InternalListContext,
    source.InternalIndex,
    assertionFactory);

If anyone constructs ListItemAtSatisfiesAssertion via the internal ctor without calling ExpressionBuilder.Append first, the expression will be silently incomplete. The factory-method approach above would eliminate this risk by co-locating both steps.

Test coverage gaps

The regression tests cover String, Collection, Dictionary, and Set for both IList and IReadOnlyList — solid baseline. Missing:

• T[] (the ArrayAssertion overload is present but untested)
• HashSet<T> (the HashSetAssertion overload exists)
• Dictionary<K,V> concrete type (there's a MutableDictionaryAssertion overload for it)

Given this is a regression fix, having at least a smoke test for each overload variant that was added would make future regressions much easier to catch.

Summary

The core fix is sound — changing the internal delegate from Func<IAssertionSource<TItem>, Assertion<TItem>?> to Func<TItem, int, IAssertion?> and building the specialized source at dispatch time is exactly right. The backward-compatible forwarding constructor preserves the existing public API cleanly.

The concern is that the mechanism chosen (enumerating all known concrete types as overloads) trades one problem (missing specialization) for another (a maintenance tax on every new assertion type). Worth considering whether a single generic sourceFactory parameter would be a better long-term design before this pattern spreads to other assertion combinators.

Code Review: Fix item-at Satisfies source typing (#5706)

The PR correctly identifies and fixes the root cause: ListItemAtSatisfiesAssertion was storing Func<IAssertionSource<TItem>, Assertion<TItem>?> which forced the lambda return type to be Assertion<TItem>? — making it impossible to use collection assertions that chain to a different Assertion<T> (e.g., Count().IsEqualTo(3) returns Assertion<int>, not Assertion<IEnumerable<int>>).

What's done well

• The internal refactor to Func<TItem, int, IAssertion?> is clean and separates item retrieval from assertion creation correctly.
• Constructor chaining preserves backward compatibility elegantly.
• The CreateSatisfiesAssertion bridge approach (following feedback from the previous review) correctly avoids exposing internal index/context accessors.
• Good test coverage across diverse concrete types.
• Public API snapshots updated properly.

Architectural Concern: The Root Fix May Be Simpler

The 303-line extension file is working around a return-type covariance constraint on the existing overload. The original signature requires Func<IAssertionSource<TItem>, Assertion<TItem>?>, but collection assertion chains return Assertion<int> (not Assertion<IEnumerable<int>>), so the lambda won't compile.

If most collection assertion extension methods (like Count()) are already defined on IAssertionSource<IEnumerable<T>> — rather than only on the concrete CollectionAssertion<T> — then adding a single overload with a relaxed return type directly on the Satisfies method would handle the majority of cases without any per-type dispatch:

// One new overload on ListItemAtSource<TList, TItem>
public ListItemAtSatisfiesAssertion<TList, TItem> Satisfies(
    Func<IAssertionSource<TItem>, IAssertion?> assertion,
    [CallerArgumentExpression(nameof(assertion))] string? expression = null)
{
    return CreateSatisfiesAssertion(
        (item, index) => assertion(new ValueAssertion<TItem>(item, $"item[{index}]")),
        expression);
}

With this, users can write item => item.Count().IsEqualTo(3) and the lambda compiles because IAssertion? is satisfied by the Assertion<int> return. The full 303-line extension file would only be needed if users specifically need the concrete assertion type (e.g., CollectionAssertion<int> has methods not available through IAssertionSource<IEnumerable<int>>).

Is the extension file truly necessary, or is the IAssertion? overload sufficient for all the test cases?

Scalability of the Extension File

If the per-type extension methods are genuinely needed, the static dispatch approach has inherent growth pressure:

• Missing today: ICollection<T>, Queue<T>, Stack<T>, LinkedList<T>, SortedList<TKey,TValue>, ImmutableArray<T>, ImmutableList<T>, etc.
• Any user-defined assertion source type will never work with this approach.

A more future-proof alternative for strongly-typed sources would be a factory overload:

public ListItemAtSatisfiesAssertion<TList, TItem> Satisfies<TSource>(
    Func<TItem, string, TSource> sourceFactory,
    Func<TSource, IAssertion?> assertion,
    [CallerArgumentExpression(nameof(assertion))] string? expression = null)
    where TSource : IAssertionSource<TItem>

Usage:

.Satisfies(
    (item, name) => new CollectionAssertion<int>(item, name),
    col => col.Count().IsEqualTo(3))

This covers all current and future assertion source types without adding overloads, at the cost of a slightly more verbose call site. The per-type extension methods could still be provided as ergonomic wrappers for the most common types.

Minor

• The string item path works correctly through the existing Func<IAssertionSource<TItem>, Assertion<TItem>?> overload (string assertions return Assertion<string>). The behavior asymmetry between string items and collection items is worth a code comment noting why no specialized overload is needed for string.
• #if NET5_0_OR_GREATER on IReadOnlySet<T> is correct; no issues there.

Overall the implementation is functionally correct and the previous review feedback has been addressed. The main open question is whether the simpler single-overload approach (Func<IAssertionSource<TItem>, IAssertion?>) would suffice, which would let you drop most of the extension file. If the specialized concrete types are genuinely needed for their extra methods, the factory overload pattern would scale better than the current per-type enumeration.

Code Review

Previous feedback (constructor chaining, CreateSatisfiesAssertion encapsulation, test coverage gaps) has been addressed. The architecture is sound.

What's working well

IAssertionSourceFor<TItem, TSelf> CRTP interface — The right foundation. Using C# 11 static abstract members for a per-item factory enables fully generic dispatch without per-type overloads at the core level. The constraint where TSource : IAssertionSourceFor<TItem, TSource> correctly closes the loop.

CreateSatisfiesAssertion encapsulation — Resolves the previous concern about leaking InternalListContext/InternalIndex. The extension methods call an internal method from within the same assembly, which is the correct scope.

Backward-compatible constructor chaining — The old public ListItemAtSatisfiesAssertion(context, index, Func<IAssertionSource<TItem>, Assertion<TItem>?>) constructor chains to the new internal one. No binary breaking change, and the wrapping via new ValueAssertion<TItem> is transparent to callers.

Func<TItem, int, IAssertion?> — Passing index alongside item is essential for constructing the label $"item[{index}]" in the right place, rather than deferring label construction to CheckAsync. This is the key fix for #5706.

Remaining design concern: List<T> / Dictionary<TKey,TValue> bridging asymmetry

The comment explains the why well, but there is a cleanable root cause. ListAssertion<T> implements IAssertionSourceFor<IList<T>, ListAssertion<T>> (interface-typed), not IAssertionSourceFor<List<T>, ListAssertion<T>> (concrete-typed). So the generic Satisfies<TSource> path can't bind when the item is List<T>, forcing the explicit bridge:

// Current — two code paths
=> source.Satisfies<ListAssertion<TInner>>(assertion, expression);   // via IAssertionSourceFor
=> source.CreateSatisfiesAssertion(                                   // manual bridge
       (item, index) => assertion(new ListAssertion<TInner>(item, ...)), expression);

ListAssertion<T> could implement both interface variants without conflict, since the Create static methods differ in parameter type (IList<T> vs List<T>):

public class ListAssertion<TItem>
    : ListAssertionBase<IList<TItem>, TItem>
#if !NETSTANDARD2_0
    , IAssertionSourceFor<IList<TItem>, ListAssertion<TItem>>   // existing
    , IAssertionSourceFor<List<TItem>, ListAssertion<TItem>>    // new — eliminates bridge
#endif
{
    public static ListAssertion<TItem> Create(IList<TItem> item, string label) => new(item, label);
    public static ListAssertion<TItem> Create(List<TItem> item, string label)  => new(item, label);
}

C# can implement two parameterizations of the same generic interface when the static abstract member signatures are distinct (different TItem → different parameter type). With this, the concrete List<T> extension overloads would reduce to simple Satisfies<ListAssertion<TInner>> calls, eliminating the asymmetric CreateSatisfiesAssertion bridge and making the code more consistent. Same pattern applies to MutableDictionaryAssertion. Not a blocker, but worth a follow-up — the inconsistency will likely surface again when users try to add their own concrete-type overloads.

Minor: extension duplication between list / readonly-list source types

Each of the 11 logical type mappings appears twice in ListItemAtSatisfiesExtensions.cs — once for ListItemAtSource<TList, TItem> and once for ReadOnlyListItemAtSource<TList, TItem>. This is intrinsic because the return types differ (ListItemAtSatisfiesAssertion vs ReadOnlyListItemAtSatisfiesAssertion). A shared interface over both source types exposing CreateSatisfiesAssertion could halve this, but the return-type variance makes that non-trivial. Noting for visibility rather than blocking the PR.

Test coverage note

Tests cover IEnumerable<int>, int[], IDictionary<string,int>, Dictionary<string,int>, ISet<int>, HashSet<int>, IReadOnlyDictionary<string,int>, IReadOnlySet<int>, and string. The IList<T> and IReadOnlyList<T> item overloads (which are among the cleanest — pure Satisfies<TSource> delegation) don't appear to have dedicated test cases. Low risk, but worth adding for completeness.

Summary

Functionally correct, all previous feedback has been addressed, and the IAssertionSourceFor CRTP foundation is genuinely extensible — users can write Satisfies<MyCustomAssertion>(...) for types TUnit doesn't know about. The main open item is the List<T>/Dictionary<TKey,TValue> concrete-type bridging asymmetry, which is cosmetic now but becomes a maintenance tax as the pattern propagates.

Code Review: Fix item-at Satisfies source typing — iteration 4

All prior feedback has been fully addressed. This review covers only the new commit (0080687e).

What changed since the last review

The latest commit does three things:

1. IAssertionSourceFor<TItem, TSelf> no longer extends IAssertionSource<TItem> — required to allow ListAssertion<T> and MutableDictionaryAssertion<T> to implement two parameterizations of the interface simultaneously.
2. ListAssertion<T> and MutableDictionaryAssertion<TKey,TValue> implement both the interface-typed and concrete-typed variants (IAssertionSourceFor<IList<T>> and IAssertionSourceFor<List<T>>), so the concrete overloads in the extension file now delegate through the generic Satisfies<TSource> path without any CreateSatisfiesAssertion bypass.
3. Test coverage added for IList<IList<T>>, IList<IReadOnlyList<T>>, IReadOnlyList<IList<T>>, and IReadOnlyList<IReadOnlyList<T>> — directly addresses the gap flagged in the last review.

All three of these were explicitly flagged as open items in the previous review, so well done on the quick turnaround.

Removing IAssertionSourceFor : IAssertionSource<TItem>

This is the right call. The commit message explains the invariance problem precisely, and the <remarks> doc on the interface explains the design rationale for future maintainers. The public API snapshot reflects the change correctly. Since IAssertionSourceFor was never shipped in a release, removing the inheritance is not a breaking change.

One small thing to verify: the API snapshots show that ArrayAssertion<TItem>, CollectionAssertion<TItem>, ReadOnlyListAssertion<TItem>, etc. now list fewer interfaces in their type hierarchy (the transitively-inherited IAssertionSource and IAssertionSource<TItem> that came from IAssertionSourceFor are no longer shown). The underlying implementations are unaffected — those interfaces are still satisfied through the base class hierarchy — so this is a snapshot artifact, not a behavior regression.

Minor gap: IList<List<T>> case not tested

The refactored List<T> overloads now route through Satisfies<ListAssertion<TInner>> (via the new IAssertionSourceFor<List<T>, ListAssertion<T>> implementation). This is the code path most changed by this commit, yet it has no dedicated test. All existing tests use IList<IList<T>>, not IList<List<T>>. A minimal test case:

[Test]
public async Task List_ItemAt_Satisfies_Preserves_ConcreteList_Item_Source()
{
    IList<List<int>> items = new List<List<int>> { new() { 1, 2, 3 } };
    await Assert.That(items).ItemAt(0).Satisfies(item => item.Count().IsEqualTo(3));
}

Same applies to IList<Dictionary<string,int>> (the concrete dictionary path). Low risk since the types are equivalent at the IList<T> / IDictionary<K,V> level, but a regression test for exactly the code path changed this round would be good practice.

Summary

The implementation is clean, the internal Satisfies<TSource> path is now used consistently for both concrete and interface item types, the CreateSatisfiesAssertion bypass is gone, and the test coverage is substantially improved. The only remaining suggestion is smoke tests for the List<T> and Dictionary<K,V> concrete-item paths that are specifically enabled by this commit. No blockers.